Add Process.Env - #930
Conversation
d0a2f4e to
10b6113
Compare
|
|
||
| module M = Map.Make(String) | ||
|
|
||
| let override bindings t = |
There was a problem hiding this comment.
This is a deviation from execve behaviour, which passes through duplicate bindings and lets glibc/musl/etc handle that. Not necessarily a bad thing though, I haven't investigated what the libcs do yet.
There was a problem hiding this comment.
I'll add some docs, but the idea is that the raw of_array API lets you do whatever you want (arbitrary array of strings), while the bindings APIs (that take lists of pairs) work like setenv (every entry added must be a key/value pair and duplicates are not created).
There was a problem hiding this comment.
This makes sense. What about case sensitivity? I think windows end keys are insensitive
There was a problem hiding this comment.
Good point. I guess the easiest way would be to change the behaviour of Process.Env depending on the host OS. I wonder what encoding Windows assumes for a case-insensitive compare? Are names always ASCII there?
There was a problem hiding this comment.
I've pushed a commit that does case insensitive compares on Windows now. It's not very efficient, but there aren't usually many variables anyway.
There was a problem hiding this comment.
The change looks good, but I really dislike the portable interface varying behaviour based on which host it's running on. Here's a radical idea: why not specify our Eio environment interface as explicitly being case insensitive? We are already constraining it to forbid duplicates, and it seems like normalising on case should also be very safe. We could also preserve the case at the Eio level (so it's passed through as-is) but is case-insensitive for comparisons.
There was a problem hiding this comment.
Being case insensitive in general seems bad: it's a surprising change to the expected behaviour, and it causes trouble with non-ascii encodings.
The other option is to track whether a particular environment is Windows-style or not. But we can't use a flag at the moment because the type needs to be string array. It would be possible to track the type with a fake entry at the start (EIO_OS=Windows or something, that gets stripped out in to_array). Ugly, though.
A simpler solution is to recommend that environment variable names are upper-case (which they mostly are anyway). As long as all variables are uppercase, the Windows and POSIX behaviours are the same anyway.
There was a problem hiding this comment.
A simpler solution is to recommend that environment variable names are upper-case (which they mostly are anyway). As long as all variables are uppercase, the Windows and POSIX behaviours are the same anyway.
agreed! good idea
This provides helper functions to get and set environment variables. For backwards compatibility the type is exposed, but having this module will make it easier to make the type abstract later. Co-authored-by: Anil Madhavapeddy <anil@recoil.org>
3 participants
This provides helper functions to get and set environment variables. For backwards compatibility the type is exposed, but having this module will make it easier to make the type abstract later.
(@avsm mentioned in #923 (comment) that we might want to change the type here at some point)
@patricoferris: this might be useful for #923.